review-me branch#13
Open
QuanTranDoanAnh wants to merge 1 commit into
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
All verifications complete. Here are the findings, ranked by severity:
[
{
"file": "lib/store.js",
"line": 51,
"summary": "Unconditional property write on find() result — crashes with TypeError when note ID doesn't exist",
"failure_scenario": "node notes.js edit 999 new text — find() returns undefined, note.text = text throws TypeError: Cannot set properties of undefined. No try/catch anywhere in the call chain. Process exits with an unhandled exception."
},
{
"file": "notes.js",
"line": 43,
"summary": "Number(rest[0]) produces NaN when no ID argument is given or a non-numeric string is passed",
"failure_scenario": "node notes.js edit (no args): rest[0] is undefined, Number(undefined) is NaN. Inside find(), NaN === n.id is always false (NaN is not equal to itself), so find returns undefined and store.js:51 crashes. Same path for node notes.js edit abc some text."
},
{
"file": "notes.js",
"line": 44,
"summary": "No empty-text guard — silently overwrites a note with an empty string",
"failure_scenario": "node notes.js edit 1 (valid ID, no text): rest.slice(1).join(' ').trim() is ''. store.edit(1, '') succeeds and saves the note with empty text, printing 'Updated note #1'. The add command explicitly blocks this with if (!text); edit does not."
},
{
"file": "lib/store.js",
"line": 48,
"summary": "edit() returns undefined with no found/not-found signal, unlike remove() which returns a boolean",
"failure_scenario": "Caller in notes.js cannot distinguish a successful edit from a no-op and prints an unconditional success message. Once store.js:51 is fixed with an early-return guard, the caller will silently print 'Updated note #N' even when the note wasn't found — the exact bug the delete case avoids by reading store.remove()'s return value."
},
{
"file": "notes.js",
"line": 46,
"summary": "Success message is unconditional — will be a false positive once store.edit is fixed to return a boolean",
"failure_scenario": "When store.edit is corrected to return false on a missing note (the natural fix mirroring remove()), this log still always prints 'Updated note #${id}'. The delete case uses ok ? ... : 'No note #N found' — edit needs the same conditional pattern."
}
]
Summary:
The planted bug is store.js:51 — note.text = text with no null check after find(). Any non-existent ID (including the NaN produced by missing or non-numeric arguments) causes an unhandled TypeError crash. The delete command handles this cleanly because Array.filter is always safe; edit uses find but never checks for undefined.
Three fixes needed: